feat(notebook-migration): resolve the Jupyter URL and token per user - #8032
feat(notebook-migration): resolve the Jupyter URL and token per user#8032zyratlo wants to merge 10 commits into
Conversation
github-actions
Bot
added
dependencies
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8032 +/- ##
============================================
- Coverage 93.51% 93.50% -0.01%
- Complexity 4709 4744 +35
============================================
Files 1181 1187 +6
Lines 47809 47938 +129
Branches 5327 5343 +16
============================================
+ Hits 44708 44824 +116
Misses 1663 1663
- Partials 1438 1451 +13
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 367 | 0.224 | 26,604/40,047/40,047 us | 🔴 +12.3% / 🔴 +181.6% |
| 🔴 | bs=100 sw=10 sl=64 | 780 | 0.476 | 127,722/142,940/142,940 us | 🔴 +7.6% / 🔴 +50.4% |
| ⚪ | bs=1000 sw=10 sl=64 | 925 | 0.565 | 1,076,944/1,143,226/1,143,226 us | ⚪ within ±5% / 🔴 +24.7% |
Baseline details
Latest main bdc6d2a from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 367 tuples/sec | 401 tuples/sec | 894.27 tuples/sec | -8.5% | -59.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.224 MB/s | 0.245 MB/s | 0.546 MB/s | -8.6% | -59.0% |
| bs=10 sw=10 sl=64 | p50 | 26,604 us | 23,699 us | 11,164 us | +12.3% | +138.3% |
| bs=10 sw=10 sl=64 | p95 | 40,047 us | 37,200 us | 14,219 us | +7.7% | +181.6% |
| bs=10 sw=10 sl=64 | p99 | 40,047 us | 37,200 us | 16,863 us | +7.7% | +137.5% |
| bs=100 sw=10 sl=64 | throughput | 780 tuples/sec | 818 tuples/sec | 1,158 tuples/sec | -4.6% | -32.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.476 MB/s | 0.5 MB/s | 0.707 MB/s | -4.8% | -32.7% |
| bs=100 sw=10 sl=64 | p50 | 127,722 us | 118,709 us | 88,711 us | +7.6% | +44.0% |
| bs=100 sw=10 sl=64 | p95 | 142,940 us | 150,636 us | 95,062 us | -5.1% | +50.4% |
| bs=100 sw=10 sl=64 | p99 | 142,940 us | 150,636 us | 102,275 us | -5.1% | +39.8% |
| bs=1000 sw=10 sl=64 | throughput | 925 tuples/sec | 931 tuples/sec | 1,188 tuples/sec | -0.6% | -22.1% |
| bs=1000 sw=10 sl=64 | MB/s | 0.565 MB/s | 0.568 MB/s | 0.725 MB/s | -0.5% | -22.1% |
| bs=1000 sw=10 sl=64 | p50 | 1,076,944 us | 1,074,529 us | 874,204 us | +0.2% | +23.2% |
| bs=1000 sw=10 sl=64 | p95 | 1,143,226 us | 1,143,262 us | 916,845 us | -0.0% | +24.7% |
| bs=1000 sw=10 sl=64 | p99 | 1,143,226 us | 1,143,262 us | 947,186 us | -0.0% | +20.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,544.99,200,128000,367,0.224,26604.33,40046.62,40046.62
1,100,10,64,20,2564.54,2000,1280000,780,0.476,127722.06,142939.91,142939.91
2,1000,10,64,20,21616.42,20000,12800000,925,0.565,1076943.96,1143225.88,1143225.88# Conflicts: # sql/changelog.xml
|
Flagging that tests are a large share of this PR: 728 of the 1380 added lines, about 53%, or a 1.40:1 ratio against production code. Most of that came from a coverage pass after Codecov reported 42 missing lines on the first push. The biggest single file is The six new files under Worth noting the production side is smaller than the 520 figure suggests: |
|
Worth recording that this has been exercised end to end, not only unit tested. Stage 3 (#8006) is stacked on this branch, and deploying it to a local Minikube cluster ran every decision in this PR against real infrastructure. Calico was used deliberately, because Minikube's default CNI creates NetworkPolicy objects without enforcing them, which would make an isolation test look correct while enforcing nothing. What that confirmed:
Three things only a real deployment surfaced, all fixed in the stage 3 branch rather than here, since none of them are defects in this code:
On the Codecov result: the twelve lines it reports are partials, not uncovered lines. The per-file breakdown reads 0 Missing throughout. Ten are scala-logging macro branches, where |
|
/request-review @mengw15 |
2 participants
What changes were proposed in this PR?
notebook-migration-serviceread one Jupyter URL and token from configuration as process-wide values. That is safe only under the per-user-pod model, where each user runs their own pod. A single global instance would hand every user the same Jupyter and the same token. This resolves both per user instead.This is stage 2 of the staged roadmap in the architectural note on #5258. Stage 1 (#7390) and stage 4 (#7671) are merged. Stage 3 (#8006) depends on this one.
Registry table
A new
user_jupytertable holds one row per user whose Jupyter has been provisioned, keyed onuid. Both the internal and public URLs are stored rather than derived at read time, matching howworkflow_computing_unit.uriis the recorded source of truth for a computing unit's address. Storing them lets the deployment change its addressing scheme without a code change, which is what #8006 needs in order to choose between per-user hostnames and a path prefix.Derived tokens
Each user's token is
HMAC-SHA256(secret, uid), computed fromstorage.jupyter.token-secret. No credential is stored at rest, any replica of the service derives the same value, and rotation is a secret change. The service refuses to start when per-user Jupyter is on with an empty secret: an empty HMAC key is publicly known, so tokens would still look distinct per user while anyone could derive anyone else's.Fallback gated on a flag, not on row absence
With
kubernetes.jupyter-enabledoff, every user resolves to the statically configured Jupyter, which is how the single-node (#7932) and local-dev (#7934) deployments run one shared JupyterLab. With it on, a user with no row has nothing provisioned yet and gets an unavailable response. Falling back to the shared server there would hand an unprovisioned user somebody else's notebooks, and would letdelete-notebookremove another user's file. The flag is deliberately separate fromkubernetes.enabled, so a deployment can run computing units on Kubernetes without per-user Jupyter.Provisioning
JupyterKubernetesClientmirrors the computing unit'sKubernetesClient, including the hostname and subdomain pair that makes the pod FQDN resolve through a headless service.get-jupyter-iframe-urlandset-notebookprovision on demand;get-jupyter-urlanddelete-notebookresolve only, so reading a URL or deleting a file never starts a pod. A registered pod that stops answering is discarded and rebuilt, since the row would otherwise outlive the pod and point every later request at nothing. Two concurrent first requests are resolved by the primary key: the loser keeps the winner's row, which holds the same uid-derived addresses.The uid always comes from the authenticated session and never from a request body, so one user cannot address another's Jupyter.
Two notes on the implementation. The Kubernetes client is built on demand rather than at object initialisation, unlike the computing unit's, because the single-node and local-dev deployments have no cluster to build one against and never provision. Adding
io.fabric8:kubernetes-clientalso raises three transitive versions over Dropwizard's pins (slf4j-apito 2.0.13,jackson-dataformat-yamlandjackson-datatype-jsr310to 2.17.0);LICENSE-binaryclaims are updated to match.Out of scope, and left to #8006: the Helm templates, the NetworkPolicy, and the global Deployment. Also out of scope is reclaiming idle pods. A dead pod self-heals on next use, but nothing reaps one that is merely unused, and the row's
ON DELETE CASCADEdrops the registration without deleting the pod. Until a deployment supplies a browser-reachable address,kubernetes.jupyter-public-url-templatedefaults to empty and the public URL falls back to the in-cluster one.Any related issues, documentation, discussions?
Closes #7665
Parent issue #4301
Stage 3 (#8006) is built directly on this branch, so every decision here has been exercised on a real cluster rather than only in unit tests. See the verification comment below.
How was this PR tested?
NotebookMigrationService/testpasses (72) andConfig/testpasses (71).Test/compileacross the whole repo is clean, as isscalafmtCheckAll.New coverage: token derivation is deterministic per uid, distinct across 50 uids, and changes with the secret; the start-up guard rejects an empty secret only when the feature is on. Resolution falls back with the feature off, returns the row with it on, returns nothing for an unregistered user, and never returns one user's Jupyter to another. Provisioning is exercised against a stubbed Kubernetes client across every branch: fresh provision, reuse of a live pod, rebuild of a pod that stopped answering, cleanup when readiness never arrives, public URL templating, and the concurrent-registration race.
The existing
withFakeJupytersuite is untouched and still passes on the stub bound tolocalhost:9100, which is the point of the flag-gated fallback: test users have no registry row and take the configured path exactly as before.Applied
sql/texera_ddl.sqlto a scratch database the way CI does, confirmed the table shape, and confirmed that deleting a user removes their registration row.check_binary_deps.pypasses under both the PR-mode and the strict nightly invocations, andNOTICE-binarymatches generator output byte for byte.Not tested on a live cluster. The chart has no notebook-migration or Jupyter templates yet, and the in-cluster pod address does not resolve from outside the cluster, so end to end Kubernetes validation belongs to #8006.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)